-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(Try to) Fix InvenioRDM license checks #222
Conversation
Note: Still not working as we get SPDX URLs but the URLs on Zenodo / InvenioRDM are opensource.org URLs
def _search_license_info(_url): | ||
for license_info in valid_licenses['hits']['hits']: | ||
try: | ||
if license_info['props']['url'] == _url: | ||
return license_info | ||
except KeyError: | ||
continue | ||
else: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Do we like nested functions?
license_info = _search_license_info(license_url) | ||
if license_info is None and license_url.startswith('https://spdx.org/licenses/'): | ||
response = requests.get(f"{license_url}.json", headers={"User-Agent": hermes_user_agent}) | ||
response.raise_for_status() | ||
|
||
for license_cross_ref in response.json()['crossRef']: | ||
if not license_cross_ref['isValid']: | ||
continue | ||
|
||
license_info = _search_license_info(license_cross_ref["url"]) | ||
if license_info is not None: | ||
break | ||
else: | ||
raise RuntimeError(f"Could not resolve license URL {license_url} to a valid identifier.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hint: Yes, this is very ugly, but seems to be robust to me, even when InvenioRDM decides to use poper SPDX URLs.
…ourney. Thanks to @zyzzyxdonta for the hint.
Question: Should we keep the Invenio implementation intact and make this the start of a new InvenioRDM plugin that then moves towards the new InvenioRDM API? |
IMHO: Well, we could keep the old plugin intact "for historical reasons" but I don't see an actual reason for it as it would be no longer used. However, a good argument for keeping the old one is to signal our numerous users that there was a change (since we do not version our plugins (yet?)). So to conclude: Yeah, let's do an InvenioRDM plugin. |
This is still based on C&P... should we extract the "common base" that is still valid and only migrate the relevant parts? Also: I stupidly changed all string occurrences... to also distinguish configurations and avoid accidential mis-use.
Zenodo was not the only Invenio instance out there 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor nitpicks 😄
nitpick: https://conventionalcomments.org states on nickpick
However, all your requested changes were well founded and I was happy to fix them. So maybe they were more todos? |
😂 Good point |
Fixes #221
Fixes #224